-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Import k8s metrics stability framework #1844
Import k8s metrics stability framework #1844
Conversation
cc @dgrisonnet |
/assign |
@@ -52,6 +56,11 @@ func NewFamilyGenerator(name string, help string, metricType metric.Type, deprec | |||
return f | |||
} | |||
|
|||
// NewFamilyGenerator creates new FamilyGenerator instances. | |||
func NewFamilyGenerator(name string, help string, metricType metric.Type, deprecatedVersion string, generateFunc func(obj interface{}) *metric.Family) *FamilyGenerator { | |||
return NewFamilyGeneratorWithStability(name, help, metricType, basemetrics.ALPHA, deprecatedVersion, generateFunc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why all metrics are being set to alpha
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all.
In replacing NewFamilyGenerator with NewFamilyGeneratorWithStability.
NewFamilyGenerator will be deleted once all replacements are done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay. So is it right to say that for now we are setting just the node
metrics to alpha
? Also, what state were they in before? Experimental?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems only two of these metrics were experimental before. So IIUC, if the node
metrics are being set to alpha
, should it only be for these two?
| kube_node_annotations | Gauge | Kubernetes annotations converted to Prometheus labels
| kube_node_role | Gauge | The role of a cluster node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what state were they in before? Experimental?
Yes, they are mapped into k8s stability level:
Experimental -> Alpha
Stable -> Stable
So IIUC, if the node metrics are being set to alpha, should it only be for these two?
Working in progress for other stable metrics. Because there are many test codes need to be changed.
I planned to create 2+ CLs. The first CL (this CL) to import stability framework, other CLs migrate other NewFamilyGenerator
to NewFamilyGeneratorWithStability
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added all STABLE metrics just now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, if everything got refactored is it possible to keep the old function name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, only stable metrics are using NewFamilyGeneratorWithStability now.
Will create another CL to migrate non-stable metrics to use NewFamilyGeneratorWithStability. Because this way has less merge conflicts in this CL.
35dec67
to
dc23fad
Compare
3e02fb7
to
1ae1e99
Compare
QQ: do we have other comments? Or could someone approve this CL? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small comment from my side, but otherwise it looks good to me. Great work @CatherineF-dev 👍
1ae1e99
to
ea90693
Compare
ea90693
to
5995c1e
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CatherineF-dev, dgrisonnet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it: #1833
How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality) No
Which issue(s) this PR fixes:
Fixes #1833